Feature/action contracts #451

krzykamil · 2024-07-22T10:22:47Z

This allows to use contact do blocks, known from dry-validation gem, in Hanami Actions (as per examples in the spec).

It is largely based on Params class, but there is less code in the Contract, since the major difference is that, Contract, delegates the work to dry-validations (Params does that too, but much later, after doing some more stuff, and it builds the schema, not the full contract, limiting the usage of rule blocks etc).

The return is still put into request.params and generally behaves the same in the end, giving more options in validations.

There is an important note, left in the code comments

# Although more complex domain-specific validations, or validations concerned with things such as uniqueness
# are usually better performed at layers deeper than your HTTP actions, Contract still provides helpful features
# that you can use without contravening the advice form above.

The contract usage possibility should be added to the documentation, I think right after this bit: https://guides.hanamirb.org/v2.1/actions/parameters/#validations-at-the-http-layer but that is up for discussion.

Any comments would be appreciated, as this code base was completely new to me, and at moments a bit overwhelming. Not sure if I did a good job with the documentation too.

timriley · 2024-08-02T14:02:53Z

Thanks @krzykamil, I'll check this out soon!

solnic

Thanks for working on this! There are a couple of things to figure out before we can merge this in.

lib/hanami/action/contract.rb

solnic · 2024-08-03T06:18:08Z

lib/hanami/action/contract.rb

+      #   end
+      # end
+      def self.contract(&blk)
+        validations(&blk || -> {})


Can't we just do @_validator = Dry::Validation::Contract.build(&blk) here and remove validations method?

You're right, it was pointless.

Thanks for the review (and while I am at it - for your OS work, that I've used extensively over the years and greatly admired). Applied all changes now.

Thanks @krzykamil 😃

timriley

This is looking good, @krzykamil!

I've had a read through of the code here and left a couple of notes.

Next step for me is to pull it down and have a proper explore and play. I'll get back to you again as soon as I can.

lib/hanami/action/contract.rb

timriley · 2024-08-06T13:22:06Z

lib/hanami/action/validatable.rb


+        # @since 2.2.0
+        # @api public
+        def contract(&blk)


Just noting that while we allow existing schemas to be passed in directly, the new code here does not support this for contracts.

Given how contracts are instances, and may also have their own dependencies, I think this is a reasonable decision, and we can always choose to reevaluate it later.

I think I understand what you want here, but I am not sure how would this look in terms of DSL.

Lets say I have a NewUserContract defined somewhere already, and I want to use it in my new action.

Should I use Deps to include this contract? Then how do I use it in the action class?
Or should I just NewUserContract do and use that in the contract method as an argument, or pass it to the block?

Asking because while using contracts as instances is not new to me, I am not sure how to implement reusability of those contracts on a class level?

I would gladly do it if I get a little bit of push in the right direction :)

Since a preexisting contract should have a container key, it would be nice to reference it by key. But that presents the problems you've described.

I think passing the contract class as an argument could be supported without much effort, but since contracts are not composable this presents a problem that params does not: passing a class would be mutually incompatible with passing a block.

Nevertheless, I still think passing a contract by class name would be useful. Otherwise if I want to share contract logic between multiple Actions or elsewhere, I must implement it multiple times.

timriley · 2024-08-07T09:27:50Z

lib/hanami/action/contract.rb

+    # that you can use without contravening the advice form above.
+    #
+    # @since 2.2.0
+    class Contract


So if I trace along the new logic in the PR:

In an action where a user defines a contract with contract do, then an instance of Hanami::Action::Contract will become the params object that we provide to the handle method as part of the request.

Am I understanding this correctly?

If so, since this params object can now be an instance of either Hanami::Action::Contract or Hanami::Action::Params, I think we want to make sure they have the same interface.

Why is this? I think it would be perfectly acceptable for a user within a single app to have params do in some action classes, and contract do in others. So this means that across a single app, the user will get different kinds of params instances, but that user should expect those objects to behave identically.

Right now Hanami::Action::Params implements a few things that the new Hanami::Action::Contract does not:

#raw

#error_messages

#deconstruct_keys

#get, #dig (via BaseParams)

#each (via BaseParams)

We need to make these consistent before we can be done with this PR.

I reckon we have a couple of options to consider here:

We could expand the Hanami::Action::Contract so that it has the same interface as Params and BaseParams

Or I wonder if there's a way we can have a single class provide this "params" interface to the user, and have it be provided the "validation" object (which is either a dry-validation contract or a dry-schema schema)? This way we can stick with a single class for params and not have to maintain two parallel implementations.

I think I like option (2) better. But I'd be keen for your thoughts!

Option 2 sounds much better

Well going with option 1 we would have easier time to change either of the implementation if there is ever a need for it, change of approach.

But for now it makes no sense to "guess" that it might happen, so I like option 2 better too.

I'll try to implement it in the next couple of days.

Thanks for this I completly missed those few methods I did wanted them to have interface as close as possible.

Thanks @krzykamil! Looking forward to seeing how you go, and of course I'm happy to provide any feedback you might need along the way.

@timriley I've pushed the changes in the last two commits.

The idea is:

Common functionality/interface can be divided into two modules:

Validations handling

Base functions

So I created two modules, included in BaseParams and ContractClass (not in Params since it inherits from BaseParams). One of them includes the conversion methods like to_h, and getters like dig, get [] etc. Those are the base functions.
The other module handles the validations parts, extracting params, handling and parsing errors, the more specific stuff connected to Dry::Validation. It also sets the same way to initialize both Params and Contract classes.

This left both Contract and Params classes as rather slim
Params only has
self._base_rules
self.params(&blk)

Those two class methods left. Contract respectiely has self.contract but also some extra stuff that Params class gets from the hanami validations gem, that I could not get around.

Also take note of the _base_rules, I did not implement an adequate method for Contract class, as I wasn't sure if we should have that there and if yes, what is the best way to do it (would appreciate a suggestion)

Anyway, let me know if this approach is OK, what else can be modified and improved.

I've copied a lot of specs from Params specs, to ensure their interfaces are indeed the same and they react similarly to similar operations

krzykamil · 2024-08-26T08:58:05Z

Just noticed that over the weekend the spec suit run and failed. I'll fix the spec today or tomorrow, this spec was ok locally so might be hard to debug, but should be done soon.

Edit: done in latest commit

timriley · 2024-08-27T07:16:57Z

Thanks @krzykamil! I've started looking at this again, btw! I want to explore whether there's an alternative approach we can take for providing the single user-exposed params interface. I'll try and get something back to you soon.

krzykamil added 6 commits June 17, 2024 16:25

First basic version - prototype

dd319d4

More details in documentation and slight refactors

47b719a

fix constant name

bb742e1

Second version, that aligns contract more with params

4e052b8

remove debug statements

c64c741

Some missing doc statements, remove useless code

ef832c6

This was referenced Jul 22, 2024

Contracts in actions krzykamil/controller#1

Closed

Allow full dry-validation contracts to be used inside actions #441

Closed

Fix rubocop offenses

59f1216

solnic requested changes Aug 3, 2024

View reviewed changes

Better names for methods, remove obsolete extraction

9debfdb

timriley reviewed Aug 6, 2024

View reviewed changes

Use ivar in to_h implementaion

56081d2

timriley reviewed Aug 7, 2024

View reviewed changes

krzykamil added 2 commits August 12, 2024 15:50

Extract common interfrace of Params and Contract

9a264e1

Final small changes

7733380

Fix confusion with #raw

3f26e7c

timriley mentioned this pull request Aug 28, 2024

Add contract support to actions #453

Merged

timriley closed this in #453 Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/action contracts #451

Feature/action contracts #451

krzykamil commented Jul 22, 2024

timriley commented Aug 2, 2024

solnic left a comment

solnic Aug 3, 2024

krzykamil Aug 5, 2024

solnic Aug 9, 2024

timriley left a comment

timriley Aug 6, 2024 •

edited

Loading

krzykamil Aug 7, 2024

alassek Aug 28, 2024

timriley Aug 7, 2024 •

edited

Loading

solnic Aug 9, 2024

krzykamil Aug 9, 2024

timriley Aug 11, 2024

krzykamil Aug 12, 2024

krzykamil commented Aug 26, 2024 •

edited

Loading

timriley commented Aug 27, 2024 •

edited

Loading

Feature/action contracts #451

Feature/action contracts #451

Conversation

krzykamil commented Jul 22, 2024

timriley commented Aug 2, 2024

solnic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley left a comment

Choose a reason for hiding this comment

timriley Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timriley Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzykamil commented Aug 26, 2024 • edited Loading

timriley commented Aug 27, 2024 • edited Loading

timriley Aug 6, 2024 •

edited

Loading

timriley Aug 7, 2024 •

edited

Loading

krzykamil commented Aug 26, 2024 •

edited

Loading

timriley commented Aug 27, 2024 •

edited

Loading